Ongoing call tiles - Implement the view-models and store#34198
Ongoing call tiles - Implement the view-models and store#34198MidhunSureshR wants to merge 20 commits into
Conversation
dbkr
left a comment
There was a problem hiding this comment.
Phew, this is quite a chunky one (I wonder if we should have landed these tiles one by one or similar).
| cli: MatrixClient; | ||
| } | ||
|
|
||
| function computeSnapshot(props: Props): MemberAvatarViewSnapshot { |
There was a problem hiding this comment.
it feels a bit weird that computeSnapshot is sometimes a private member function (FacePileViewModel) and sometimes an independent function of the props. I wonder if we should be consistent.
There was a problem hiding this comment.
I would prefer it to always be a private member function but sometimes you need to calculate the initial snapshot before you can call any methods of the vm (because you need to pass it to super()). So far we've treated this as an implementation detail.
There was a problem hiding this comment.
ah right. I wonder if we could make it always a separate function. Not something for now though.
| T extends CommonOngoingCallTileViewSnapshot = CommonOngoingCallTileViewSnapshot, | ||
| > extends BaseViewModel<T, Props> { | ||
| public constructor(props: Props, extraSnapshot: Partial<T> = {}) { | ||
| const snapshot = { ...computeSnapshot(props), ...extraSnapshot }; |
There was a problem hiding this comment.
computeSnapshot throws a few exceptions: are they handled by whatever constructs this and can we document this as throwing them?
There was a problem hiding this comment.
These errors are unlikely to be thrown:
- The
!roomIdthrow is unlikely to happen becauseMatrixEvent.getRoomId()only returns undefined for presence events. - The
!startedUserIdthrow is unlikely to happen because all events have a sender and the undefined type is an implementation quirk from js-sdk. - The
!roomthrow is unlikely to happen because you wouldn't end up in this code without the room existing. - The
!startedMemberthrow is unlikely to happen becauseMatrixEvent.senderis only undefined for presence events.
But we do have some designs for an error tile so happy to do a follow up PR that collects the errors and renders them out in the timeline.
There was a problem hiding this comment.
If they're unlikely then it's probably fine if they're caught by the event tile error boundary
| const call = getCallInRoom(this.props.callStore, this.props.roomId); | ||
| this.disposables.trackListener(call, CallEvent.Participants, ((participants: Map<RoomMember, Set<string>>) => { | ||
| this.onParticipantsChange(participants); | ||
| }) as (...args: unknown[]) => void); |
There was a problem hiding this comment.
The casts here (and above) seem a bit weird
There was a problem hiding this comment.
It is but this is a typing issue in the base view model code. Will fix separately.
| function isCallDeclinedByOwnUser( | ||
| notificationEvent: MatrixEvent, | ||
| getRelationsForEvent: GetRelationsForEvent | undefined, | ||
| cli: MatrixClient, | ||
| ): boolean { | ||
| const declinedEvents = getDeclinedEvents(notificationEvent, getRelationsForEvent); | ||
| const ownUserId = cli.getUserId(); | ||
| return declinedEvents?.some((event) => event.getSender() === ownUserId) ?? false; | ||
| } |
There was a problem hiding this comment.
I'm sort of surprised tools like this aren't available in the call code itself: it feels quite generally useful, but I guess it can always be moved if needed elsewhere.
dbkr
left a comment
There was a problem hiding this comment.
Thanks for the changes, looks sensible I think
Branched from #34197
This PR implements the view-models necessary for driving the views. There's no foolproof way to associate a given call to a rtc notification event (which is what we render as tiles) so the basic idea is to treat the last notification tile in the timeline as an ongoing call tile if there's a call in the room.
BaseOngoingCallTileViewModelthat provides the shared snapshot content.DmOngoingCallTileViewModelandRoomOngoingCallTileViewModelboth extend this base view model to provide the part of the snapshot that is not common.FacePileViewModel,MemberAvatarViewModelandDurationViewModelfor driving the views from the last PR.LatestRtcNotificationEventStore, that tracks the latest notification event in a given room. This is necessary because the call store tells you about a new call as soon as a call membership event comes through but the timeline only renders rtc notification events. The problem arises when there's a delay between receiving the call membership event and the notification event. As a result,CallStoretells us that a call is ongoing in the room but the corresponding rtc notification event hasn't arrived, so we incorrectly show the last call tile (which in reality relates to a call that is already over) as ongoing.